Fix[mqb]: FileStore must flush storage before control messages - #1202
Conversation
88eb253 to
6aff220
Compare
dorjesinpo
changed the title
6aff220 to
a9e21b4
Compare
| BALL_LOG_ERROR << description() << " teardown " | ||
| << d_clusterData.membership().clusterNodeSessionMap().size() | ||
| << " ClusterNodeSession"; | ||
|
|
There was a problem hiding this comment.
| BALL_LOG_ERROR << description() << " teardown " | |
| << d_clusterData.membership().clusterNodeSessionMap().size() | |
| << " ClusterNodeSession"; |
Should not be an error log
| int partitionId) | ||
| void StorageUtil::transitionToActivePrimary(PartitionInfo* partitionInfo, | ||
| mqbs::FileStore* fs, | ||
| int partitionId) |
There was a problem hiding this comment.
| int partitionId) | |
| ) |
I am curious why don't we make partitionId a field in FileStore, we can use it here (extract from fs).
Current API allows to work with FileStore with provided incorrect partitionId
Also there are different members in FileStore that require partitionId arg
There was a problem hiding this comment.
If you agree with this, I can do it in another PR
| @@ -545,9 +545,9 @@ struct StorageUtil { | |||
There was a problem hiding this comment.
The comment still mentions clusterData
| @@ -14,7 +14,7 @@ | |||
| // limitations under the License. | |||
|
|
|||
| // mqbc_controlmessagetransmitter.cpp -*-C++-*- | |||
There was a problem hiding this comment.
| // mqbc_controlmessagetransmitter.cpp -*-C++-*- | |
| // mqbnet_controlmessagetransmitter.cpp -*-C++-*- |
| // executed by the cluster *DISPATCHER* thread | ||
|
|
||
| // PRECONDITIONS | ||
| BSLS_ASSERT_SAFE(d_cluster_p->inDispatcherThread()); |
There was a problem hiding this comment.
d_schemaBuilder is not thread safe, and we are lifting thread restrictions by removing this line. Do I get it right that cluster owns its ControlMessageTransmitter exclusively, and each FileStore own its copy exclusively?
Would be nice to have a same thread checker if we don't know who is the owner. Or we can provide a DispatcherClient pointer arg to constructor and verify thread against it
There was a problem hiding this comment.
We are downgrading this component to a lower level where the responsibility for checking is on the caller. It is becoming simpler. Like, we do not check thread safety in the builder.
| #ifndef INCLUDED_MQBNET_CONTROLMESSAGETRANSMITTER | ||
| #define INCLUDED_MQBNET_CONTROLMESSAGETRANSMITTER | ||
|
|
||
| /// @file mqbc_controlmessagetransmitter.h |
There was a problem hiding this comment.
| /// @file mqbc_controlmessagetransmitter.h | |
| /// @file mqbnet_controlmessagetransmitter.h |
I think there are other places with mqbc
| @@ -24,7 +24,7 @@ | |||
| /// @bbref{mqbc::ControlMessageTransmitter} provides a mechanism to transmit | |||
There was a problem hiding this comment.
| /// @bbref{mqbc::ControlMessageTransmitter} provides a mechanism to transmit | |
| /// @bbref{mqbnet::ControlMessageTransmitter} provides a mechanism to transmit |
002cf7d to
d68718c
Compare
| d_clusterData.messageTransmitter().broadcastMessage( | ||
| controlMsg, | ||
| d_clusterData.transportManager()); | ||
|
|
There was a problem hiding this comment.
Trailing spaces
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
d68718c to
8babee8
Compare
|
IT failures are unrelated and being fixed |
2 participants
When sending Recovery control message, must flush the storage builder. Otherwise, data may arrive after relevant FSM event out of order.
First step is to move
ControlMessageTransmitterfrommqbctomqbnetThen, let
FileStorehave it and use it for transmitting control messagesThen, make the transmitter flush
FileStore::d_storageEventBuilder